Skip to content

Refactor FXIOS-12796 [Swift 6 Migration] Isolate SearchEnginesManager to the main actor #30353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Refactor FXIOS-12796 [Swift 6 Migration] Isolate SearchEnginesManager to the main actor #30353

wants to merge 5 commits into from
+135 −39

Conversation

@ih-codes
Copy link
Collaborator

@ih-codes ih-codes commented Nov 3, 2025 •
edited
Loading

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR isolates the search engines manager to the main actor. I noticed there was a comment in the class that the class is not "thread safe" and should be only accessed from a single thread... so this made sense to me:

/// This class is not thread-safe -- you should only access it on a single thread (usually, the main thread)!

This necessitated updating several call sites. I also opted to migrate the TelemetryWrapper (which accesses the defaultEngine) to use Notifiable.

cc @Cramsden @lmarceau @dataports | Swift 6 Migration

📝 Checklist

  • I filled in the ticket numbers and a description of my work
  • I updated the PR name to follow our PR naming guidelines
  • I ensured unit tests pass and wrote tests for new code
  • If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
  • If adding telemetry, I read the data stewardship requirements and will request a data review
  • If adding or modifying strings, I read the guidelines and will request a string review from l10n
  • If needed, I updated documentation and added comments to complex code

@ih-codes ih-codes requested a review from Cramsden November 3, 2025 21:55
@ih-codes ih-codes requested a review from a team as a code owner November 3, 2025 21:55
@ih-codes
Copy link
Collaborator Author

ih-codes commented Nov 3, 2025

Will fix up the unit tests tomorrow. I need to decide if I want to isolate the helper methods or use async.

cc @mattreaganmozilla Just in case you see any concerns with marking the SearchEnginesManager @MainActor. I noticed a comment on the class said it should be accessed only from one thread anyway, so I think this should be fine.

@ih-codes
Copy link
Collaborator Author

ih-codes commented Nov 3, 2025

@Cramsden Might be easier to review if you look at the commits since a lot of test files had updates that aren't really important.

Let me know if you think there's a better way to handle the isolated search engines manager and/or the defaultEngine. I largely just brute forced through most of the unit test isolation "fixes". 😕

@mobiletest-ci-bot
Copy link

mobiletest-ci-bot commented Nov 4, 2025

Messages
📖 Project coverage: 38.17%

💪 Quality guardian

15 tests files modified. You're a champion of test coverage! 🚀

🥇 Perfect PR size

Smaller PRs are easier to review. Thanks for making life easy for reviewers! ✨

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

✅ Per-file coverage

All changed files meet the threshold of 35.0%.

Client.app: Coverage: 37.05

File Coverage
TopSitesViewModel.swift 33.48% ⚠️
TopSitesDataAdaptor.swift 96.06% ✅
TopSitesManager.swift 98.03% ✅
TopSiteHistoryManager.swift 85.0% ✅
SearchEnginesManager.swift 88.28% ✅
TelemetryWrapper.swift 63.69% ✅
AddressToolbarContainerModel.swift 87.3% ✅

Generated by 🚫 Danger Swift against d531711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Cramsden Cramsden Awaiting requested review from Cramsden

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ih-codes @mobiletest-ci-bot